Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cost arbitrator called twice #patch #77

Merged
merged 3 commits into from
Nov 19, 2024

Conversation

ll-nick
Copy link
Collaborator

@ll-nick ll-nick commented Nov 19, 2024

Closes #62

The CostArbitrator calls getCommand twice. Once in getAndVerifyCommand like all verifier types but also once in sortOptionsByGivenPolicy since the command is required to estimate the cost.
The call to getCommand can potentially be quite expensive, though. Therefore, this PR adds a cache for it inside the Arbitrator::Option class. When computing a command, the arbitrator now doesn't call the behavior directly anymore but instead calls the wrapper inside the Option class. Should the function be called two or more times inside a single time step, the cached command is returned and the behavior is not called again.

@ll-nick ll-nick requested a review from orzechow November 19, 2024 07:15
@ll-nick ll-nick self-assigned this Nov 19, 2024
Copy link
Member

@orzechow orzechow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! 👍

test/cost_arbitrator.cpp Show resolved Hide resolved
@ll-nick ll-nick merged commit 53d764b into main Nov 19, 2024
1 check passed
@ll-nick ll-nick deleted the fix_cost_arbitrator_called_twice branch November 19, 2024 14:51
@ll-nick
Copy link
Collaborator Author

ll-nick commented Jan 8, 2025

@orzechow Just FYI:

We ran into an issue because of this. I wouldn't necessarily say that it's an error in the library but something to be aware of when using it. When using a behavior component with a cost arbitrator, this change will result in the following order of calls on the behavior component, when it has never been called before:

During the sorting of the cost estimators options:

  1. gainControl
  2. getCommand (it's actually computed here)
  3. loseControl

When actually calling the command:

  1. gainControl
  2. getCommand (the cached version)

In our specific behavior component, we set a class variable required for the commitment condition during getCommand which was reset again during loseControl. Since getCommand is now just returning the cached value, the variable was not set again which broke the behavior's commitment.

I suppose having these side effects in the getCommand function is not very clean anyway so I now solved that differently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getCommand is being called twice in CostArbitrator
2 participants